feat: add number/float16/base/assert/is-almost-same-value#9664
Conversation
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: passed
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Coverage Report
The above coverage report was generated for the changes in this PR. |
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
Planeshifter
left a comment
There was a problem hiding this comment.
Implementation differs in behavior from float32 and float64.
| // Handle signed zeros (-0 !== +0 in SameValue): | ||
| if ( isPositiveZero( af ) && isNegativeZero( bf ) ) { | ||
| return false; | ||
| } | ||
| if ( isNegativeZero( af ) && isPositiveZero( bf ) ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This signed zero handling behaves differently from the float32 and float64 equivalents.
The float32 is-almost-same-value only distinguishes between +0 and -0 when maxULP === 0. When maxULP >= 1, it returns true for (0.0, -0.0) because they're 0 ULPs apart (per ulp-difference).
Currently this implementation always returns false for +0 vs -0 regardless of maxULP, which creates an inconsistency:
// float32 behavior:
isAlmostSameValue(0.0, -0.0, 0); // false
isAlmostSameValue(0.0, -0.0, 1); // true
// float16 behavior (this PR):
isAlmostSameValue(0.0, -0.0, 0); // false
isAlmostSameValue(0.0, -0.0, 1); // false (inconsistent!)Check the float32 and float64 implementations for what is expected.
There was a problem hiding this comment.
@Planeshifter I noticed that the float32 implementation uses isSameValue as a dependency:
Should I:
Implement is-same-value for float16 first, then update this PR to match the float32 pattern exactly? This would maintain consistency across the codebase.
Keep the current self-contained implementation that uses the 1/x approch directly for signed zeros without depending on is-same-value?
| tape( 'the function returns `false` if signed zeros are provided as input values irrespective of the specified number of ULPs', function test( t ) { | ||
| t.strictEqual( isAlmostSameValue( 0.0, -0.0, 0 ), false, 'returns expected value' ); | ||
| t.strictEqual( isAlmostSameValue( -0.0, 0.0, 0 ), false, 'returns expected value' ); | ||
| t.strictEqual( isAlmostSameValue( 0.0, -0.0, 1 ), false, 'returns expected value' ); | ||
| t.strictEqual( isAlmostSameValue( -0.0, 0.0, 1 ), false, 'returns expected value' ); | ||
| t.end(); | ||
| }); |
There was a problem hiding this comment.
The test description and expectations for maxULP >= 1 don't match the float32 equivalent's behavior. The float32 version returns true when comparing +0 and -0 with maxULP >= 1, because they're 0 ULPs apart.
If aligning with float32, these tests should be:
| tape( 'the function returns `false` if signed zeros are provided as input values irrespective of the specified number of ULPs', function test( t ) { | |
| t.strictEqual( isAlmostSameValue( 0.0, -0.0, 0 ), false, 'returns expected value' ); | |
| t.strictEqual( isAlmostSameValue( -0.0, 0.0, 0 ), false, 'returns expected value' ); | |
| t.strictEqual( isAlmostSameValue( 0.0, -0.0, 1 ), false, 'returns expected value' ); | |
| t.strictEqual( isAlmostSameValue( -0.0, 0.0, 1 ), false, 'returns expected value' ); | |
| t.end(); | |
| }); | |
| tape( 'the function distinguishes between signed zeros when the specified number of ULPs is zero', function test( t ) { | |
| t.strictEqual( isAlmostSameValue( 0.0, -0.0, 0 ), false, 'returns expected value' ); | |
| t.strictEqual( isAlmostSameValue( -0.0, 0.0, 0 ), false, 'returns expected value' ); | |
| t.strictEqual( isAlmostSameValue( 0.0, -0.0, 1 ), true, 'returns expected value' ); | |
| t.strictEqual( isAlmostSameValue( -0.0, 0.0, 1 ), true, 'returns expected value' ); | |
| t.end(); | |
| }); |
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | ||
| var f16 = require( '@stdlib/number/float64/base/to-float16' ); | ||
| var ulpdiff = require( '@stdlib/number/float16/base/ulp-difference' ); |
There was a problem hiding this comment.
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | |
| var f16 = require( '@stdlib/number/float64/base/to-float16' ); | |
| var ulpdiff = require( '@stdlib/number/float16/base/ulp-difference' ); | |
| var isnan = require( '@stdlib/number/float16/base/assert/is-nan' ); | |
| var isSameValue = require( '@stdlib/number/float16/base/assert/is-same-value' ); | |
| var ulpdiff = require( '@stdlib/number/float16/base/ulp-difference' ); |
Handling -+0 and isnan should come form isSameValue
Should be resolved once #9677 is merged.
| var af; | ||
| var bf; | ||
|
|
||
| af = f16( a ); | ||
| bf = f16( b ); | ||
|
|
||
| // Check for NaN (NaN is considered equal to NaN in SameValue): | ||
| if ( isnan( af ) && isnan( bf ) ) { | ||
| return true; | ||
| } | ||
| // Only distinguish signed zeros when maxULP is 0: | ||
| if ( maxULP === 0 && af === 0.0 && bf === 0.0 ) { | ||
| return ( 1.0 / af === 1.0 / bf ); | ||
| } | ||
| // Check if exactly equal: | ||
| if ( af === bf ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
| var af; | |
| var bf; | |
| af = f16( a ); | |
| bf = f16( b ); | |
| // Check for NaN (NaN is considered equal to NaN in SameValue): | |
| if ( isnan( af ) && isnan( bf ) ) { | |
| return true; | |
| } | |
| // Only distinguish signed zeros when maxULP is 0: | |
| if ( maxULP === 0 && af === 0.0 && bf === 0.0 ) { | |
| return ( 1.0 / af === 1.0 / bf ); | |
| } | |
| // Check if exactly equal: | |
| if ( af === bf ) { | |
| return true; | |
| } | |
| if ( isnan( a ) || isnan( b ) || maxULP === 0 ) { | |
| return isSameValue( a, b ); | |
| } |
Better to use the explicit float16 version of isnan rather than converting to half-precision
| var bool = isAlmostSameValue( 1.0, 1.0+EPS, 1 ); | ||
| console.log( bool ); | ||
| // => true | ||
|
|
||
| bool = isAlmostSameValue( 1.0+EPS, 1.0, 1 ); | ||
| console.log( bool ); | ||
| // => true | ||
|
|
||
| bool = isAlmostSameValue( 1.0, 1.0+EPS+EPS, 1 ); | ||
| console.log( bool ); | ||
| // => false | ||
|
|
||
| bool = isAlmostSameValue( 1.0, 1.0+EPS, 0 ); | ||
| console.log( bool ); | ||
| // => false | ||
|
|
||
| bool = isAlmostSameValue( -0.0, 0.0, 0 ); | ||
| console.log( bool ); | ||
| // => false | ||
|
|
||
| bool = isAlmostSameValue( 1.0, NaN, 1 ); | ||
| console.log( bool ); | ||
| // => false | ||
|
|
||
| bool = isAlmostSameValue( NaN, NaN, 1 ); | ||
| console.log( bool ); | ||
| // => true | ||
| ``` |
There was a problem hiding this comment.
| var bool = isAlmostSameValue( 1.0, 1.0+EPS, 1 ); | |
| console.log( bool ); | |
| // => true | |
| bool = isAlmostSameValue( 1.0+EPS, 1.0, 1 ); | |
| console.log( bool ); | |
| // => true | |
| bool = isAlmostSameValue( 1.0, 1.0+EPS+EPS, 1 ); | |
| console.log( bool ); | |
| // => false | |
| bool = isAlmostSameValue( 1.0, 1.0+EPS, 0 ); | |
| console.log( bool ); | |
| // => false | |
| bool = isAlmostSameValue( -0.0, 0.0, 0 ); | |
| console.log( bool ); | |
| // => false | |
| bool = isAlmostSameValue( 1.0, NaN, 1 ); | |
| console.log( bool ); | |
| // => false | |
| bool = isAlmostSameValue( NaN, NaN, 1 ); | |
| console.log( bool ); | |
| // => true | |
| ``` | |
| var bool = isAlmostSameValue( 1.0, 1.0+EPS, 1 ); | |
| // returns true | |
| bool = isAlmostSameValue( 1.0+EPS, 1.0, 1 ); | |
| // returns true | |
| bool = isAlmostSameValue( 1.0, 1.0+EPS+EPS, 1 ); | |
| // returns false | |
| bool = isAlmostSameValue( 1.0, 1.0+EPS, 0 ); | |
| // returns false | |
| bool = isAlmostSameValue( -0.0, 0.0, 0 ); | |
| // returns false | |
| bool = isAlmostSameValue( 1.0, NaN, 1 ); | |
| // returns false | |
| bool = isAlmostSameValue( NaN, NaN, 1 ); | |
| // returns true |
Let's remove the console.log in the markdown.
There was a problem hiding this comment.
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves none.
Description
This pull request:
number/float16/base/assert/is-almost-same-valueRelated Issues
This pull request has the following related issues:
Questions
No.
Other
No.
Checklist
AI Assistance
@stdlib-js/reviewers